-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add utility methods to User class #1259
base: main
Are you sure you want to change the base?
Conversation
I feel This allows users to use an auth handler class (which isn't an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @divyansshhh - thanks for opening this pull request.
I agree with the idea of moving these to the User
class, especially if developers find that more convenient. I do have some comments about the approach that I've included below.
Thank you for the suggestion, I've made the required changes. |
"""Serialize a user to a string for storage in a cookie | ||
If overriding in a subclass, make sure to define from_string as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not assume this is for cookie usage...
"""Serialize a user to a string for storage in a cookie | |
If overriding in a subclass, make sure to define from_string as well. | |
"""Serialize a user to a string | |
If overriding in a subclass, make sure to define the class method from_string as well. |
Default is just the user's username. | ||
""" | ||
# default: username is enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how this comment comes into play here. Is the help string and comment valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment and docstring are out of date, behavior changed in #863.
Default is just the user's username. | ||
""" | ||
# default: username is enough | ||
cookie = json.dumps( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No cookie...
cookie = json.dumps( | |
serialized_user = json.dumps( |
|
||
@classmethod | ||
def from_string(cls, serialized_user: str) -> User: | ||
"""Inverse of user_to_cookie""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Inverse of user_to_cookie""" | |
"""Creates an instance of User from a serialized user instance (the inverse of to_string)""" |
def from_string(cls, serialized_user: str) -> User: | ||
"""Inverse of user_to_cookie""" | ||
user = json.loads(serialized_user) | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the try/except necessary? Since loads
can raise as well, we might as well let things fly (no try/except) unless we want to introduce a custom exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this is a breaking change because IdentityProviders defining their own cookie storage will no longer have any effect, they will instead have to move their implementation to a custom User subclass.
I was checking with JupyterHub, which does indeed change the cookie serialization (only a token is stored, not any part of the user model), but it does so at the get_user
level, so these methods are not used.
I think it's also relevant to keep the notion of serialization for cookie purposes, which is what these methods are really for. I guess it doesn't have to be specific to cookies, but any persistence mechanism, but the sole point of these methods is serialization of users to allow later reconstruction by this class. Since it can and often should include things like credentials, we should be clear that the serialized form shouldn't be used for logs, etc. or anything else other than passing to from_string
.
Default is just the user's username. | ||
""" | ||
# default: username is enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment and docstring are out of date, behavior changed in #863.
Good point. I was too focused on the idea of moving serialization to the |
We do actually have that, by extending get_user. Subclassing User is mentioned in the get_user docstring. JupyterHub relies on it. Ultimately, I don't think is matters so much on which class the method lives, since it's the IdentityProvider that is responsible for selecting and calling it either way - it just means the User class and the IdentityProvider are even more deeply coupled. If the IdentityProvider wants to customize how a logged-in user is persisted (the identity provider's responsibility), it must:
So this change currently makes it a bit more work to change user serialization. That would be mitigated if While it's a breaking change in the strictest sense (removal of an API), the only case I know of (JupyterHub) that modifies exactly this behavior (how a User is persisted in a cookie), does so at a higher level, so it's unaffected by the change here. Really only cases of custom user models that want to preserve default cookie persistence should be affected, and I'm not sure they exist yet. |
No description provided.